Skip to content

Prefer Utxo::Local over Utxo::Foreign in OldestFirstCoinSelection#265

Merged
oleonardolima merged 1 commit intobitcoindevkit:masterfrom
nymius:fix/issue-264-oldest-first-selects-local-last
Jul 1, 2025
Merged

Prefer Utxo::Local over Utxo::Foreign in OldestFirstCoinSelection#265
oleonardolima merged 1 commit intobitcoindevkit:masterfrom
nymius:fix/issue-264-oldest-first-selects-local-last

Conversation

@nymius
Copy link
Contributor

@nymius nymius commented Jun 11, 2025 •
edited
Loading

Description

The comments in the OldestFirstCoinSelection implementation stated the following:

// For utxo that doesn't exist in DB, they will have lowest priority to be selected

But this was not honored in the code.

This PR enforces this and ensures the expected behaviour through the two following new tests:

  • test_oldest_first_coin_selection_uses_all_optional_with_foreign_utxo_locals_sorted_first
  • test_oldest_first_coin_selection_uses_only_all_optional_local_utxos_not_a_single_foreign

Fixes #264

Changelog notice

No public APIs are changed by these commits.

Checklists

Important

This pull request DOES NOT break the existing API

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo +nightly fmt and cargo clippy before committing
  • I've added tests for the new code
  • I've expanded docs addressing new code
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@nymius nymius self-assigned this Jun 11, 2025
@coveralls
Copy link

coveralls commented Jun 11, 2025 •
edited
Loading

Pull Request Test Coverage Report for Build 15949711702

Details

  • 68 of 68 (100.0%) changed or added relevant lines in 1 file are covered.
  • 14 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.9%) to 84.663%
Files with Coverage Reduction New Missed Lines %
wallet/src/descriptor/dsl.rs 1 95.34%
wallet/src/wallet/changeset.rs 2 85.0%
wallet/src/descriptor/policy.rs 3 79.07%
wallet/src/descriptor/template.rs 4 98.04%
wallet/src/wallet/mod.rs 4 81.04%
Totals Coverage Status
Change from base Build 15719064446: -0.9%
Covered Lines: 6525
Relevant Lines: 7707
💛 - Coveralls

@MaxFangX
Copy link
Contributor

MaxFangX commented Jun 11, 2025

Thanks for the quick fix!

evanlinjin
evanlinjin reviewed Jun 12, 2025
View reviewed changes
@nymius nymius force-pushed the fix/issue-264-oldest-first-selects-local-last branch 2 times, most recently from ef5cf30 to d23d07b Compare June 12, 2025 22:57
@nymius
Copy link
Contributor Author

nymius commented Jun 12, 2025

@MaxFangX thanks for looking at the code so close!
Just for clarification, if you're using TxBuilder to create transactions, the only way to have foreign UTxOs in your selection is through the add_foreign_utxo method, which adds UTxOs to the manual selected list. That list would be passed to the coin selection implementation as the required_utxos. So, there is no way through that API to stumble upon this issue.
If you are not using TxBuilder::create_tx, then you could create your optional_utxos with as many foreign UTxOs as you want, and in that case the outcome would be affected.

@nymius nymius added this to the Wallet 2.1.0 milestone Jun 17, 2025
@nymius nymius requested a review from evanlinjin June 18, 2025 02:12
evanlinjin
evanlinjin approved these changes Jun 19, 2025
View reviewed changes
Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK d23d07b

@ValuedMammal ValuedMammal moved this to Needs Review in BDK Wallet Jun 23, 2025
@notmandatory notmandatory added the bug Something isn't working label Jun 25, 2025
@nymius nymius force-pushed the fix/issue-264-oldest-first-selects-local-last branch from d23d07b to 500120c Compare June 26, 2025 22:22
oleonardolima
oleonardolima approved these changes Jun 27, 2025
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 500120c

oleonardolima
oleonardolima reviewed Jun 27, 2025
View reviewed changes
@nymius nymius force-pushed the fix/issue-264-oldest-first-selects-local-last branch from 500120c to 345cc6e Compare June 29, 2025 00:33
ValuedMammal
ValuedMammal approved these changes Jul 1, 2025
View reviewed changes
Copy link
Collaborator

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 345cc6e

oleonardolima
oleonardolima approved these changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 345cc6e

@oleonardolima oleonardolima merged commit d0ba53c into bitcoindevkit:master Jul 1, 2025
20 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Wallet Jul 1, 2025
@nymius nymius deleted the fix/issue-264-oldest-first-selects-local-last branch July 1, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evanlinjin evanlinjin evanlinjin approved these changes

@oleonardolima oleonardolima oleonardolima approved these changes

@ValuedMammal ValuedMammal ValuedMammal approved these changes

Assignees

@nymius nymius

Labels

bug Something isn't working

Projects

BDK Wallet
Archived in project

Milestone

Wallet 2.1.0

Development

Successfully merging this pull request may close these issues.

OldestFirstCoinSelection selects Utxo::Foreigns first instead of last

7 participants

@nymius @coveralls @MaxFangX @evanlinjin @oleonardolima @ValuedMammal @notmandatory